FIX(client): Fix using the keyboad to change local volume adjustment #6238

Hartmnt · 2023-10-11T18:55:28Z

Previously, when using keyboard arrows, the new slider widget action applied the new local volume but did not save it. That was because it used the sliderReleased event to save the value, which is not triggered by keyboard updates.

We could simply save the local volume on every sliderChange instead of on sliderRelease, but that would cause many writes to the disk in a short period of time and possibly lag the client.

This commit introduces an event filter in addition to the sliderChanged event, which is observing key releases. If the slider has focus and left/right is released, it is then calling the save slot of the slider.

Fixes #6211

Hartmnt · 2023-10-11T22:32:17Z

Ahhrg, the problem still exists when scrolling over a focused slider with the mouse...

Krzmbrzl

Another thing to keep in mind here is that for volume changes for listeners, every change will trigger a message to be sent to the server (with newer versions of Mumble) as the listener volumes are (partially) handled server-side.
Therefore, I think we'll want some kind of delay before triggering changeCompleted to see whether the user is currently simply smashing the arrows keys to get the slider to the position that they want and only once they have settled on a given value (for e.g. half a second) do we actually trigger the event.

src/mumble/widgets/EventFilters.h

Hartmnt · 2023-10-16T20:54:53Z

Another thing to keep in mind here is that for volume changes for listeners, every change will trigger a message to be sent to the server (with newer versions of Mumble) as the listener volumes are (partially) handled server-side.
Therefore, I think we'll want some kind of delay before triggering changeCompleted to see whether the user is currently simply smashing the arrows keys to get the slider to the position that they want and only once they have settled on a given value (for e.g. half a second) do we actually trigger the event.

I do not get this to work in a nice user friendly way. It either feels laggy, or does not update the user state correctly. I tried to implement bursts, but it does not help. Any advice?

Hartmnt · 2023-10-20T12:21:31Z

After more testing I figured out, why this feels currently laggy. For some reason - investigation pending - the server does not acknowledge every change of the listener volume adjustment. This can only be reasonably noticed with a client that prints the decibel value next to the user, as you can see the number is not being updated properly.

I have added a screenshot which shows the problem: The client sends the new adjustment, but there is no ack (or rather listener update) coming from the server. Is the server rejecting fast client changes anyway? That would make it really hard for us to implement proper burst behavior without knowing the server configuration...

This should also be a problem in current versions of Mumble, if you slide the slider fast enough with the mouse.

Hartmnt · 2023-10-24T18:47:32Z

Ok here is my new take on the timer:

I am running into the server message burst limit in my testing whether I use a timer or not. The Mumble server already restricts the control messages it accepts from a client. It should not be the burden of any individual feature of the Mumble client to restrict the message count sent to the server, except for trivial cases.

Therefore I propose we

accept that repeatedly pressing arrow keys will produces more messages than using the mouse
note that the server has a message burst limit already implemented and configurable by the admin
Open a new feature-request issue to extend the Mumble protocol such that the server can inform the client about its burst limits and implement control message queuing in the client...

Krzmbrzl · 2023-10-25T07:13:53Z

implement control message queuing in the client

This in and of itself would not really solve the problem though. It would prevent the client from running into the rate limiter, but it could also lead to situations where all kinds of actions appear super, super laggy just because the user has spammed the arrow key in the volume adjustment dialog (or something similar).
We would have to implement a client-side mechanism to discard certain control messages, if there is a newer one that overwrites the old one. And figuring that out properly seems quite challenging. Not sure if that would be really worth it.

I would rather opt for the laggy behavior when using arrow keys 👀

What was the timer delay that you have used before sending out a message to the server about the new listener volume? By default the server accepts 1 control message per second in the long run and 5 messages per second for short bursts.
Therefore, if we restrict ourselves to about one message per second for the volume adjustments, we should never run into the rate limiter.

In order to make the lag less apparent, we could use something like a capped exponential growth for determining the current delay: E.g. the first time the arrow key is pressed, we send a message immediately. The second time, we wait for something like 250ms and if we register a third key press, we push the timeout to 1s.
If the last key press was more than x seconds ago, we reset the counter press counter and start again.

The timeout before updating the actual adjustment could also be indicated in the UI so that the user gets informed about it.

Alternatively, we could simply always set the timeout between key presses to e.g. 750ms, which should make sure that unless a user is deliberately trying to hit the rate limit, they won't run into it. This would make all volume updates via key presses somewhat laggy, but I think that would be a reasonable compromise between effort of implementation and UX. Maybe the timeout could even be lowered a bit further. This would need some testing.

We only have to make sure that if the last UI change has not (yet) led to an update sent to the server, we send that update upon closing the dialog. This should ensure that the adjustments are always communicated to the server, regardless of how quickly the user closes the dialog again.

Hartmnt · 2023-11-01T15:22:08Z

Alright, so first I implemented a new timer in the ListenerVolumeSlider similar to what you suggested. It works now, it is not an excellent UX, but it works. It also updates the volume after you close the QMenu.

But - let me tell you - I have spent countless hours implementing a mouse wheel event observer. And I have just now realized, that it was buggy Qt all along that threw me off. Guess what? Qt mouse wheel behavior is entirely non-deterministic. When you start up the app you have the chance to be in one of 3 or so states which each will produce completely different Qt::ScrollPhase types on the same inputs. Some of these values are not even valid enum entries in Qt::ScrollPhase (7 and 5) which is absolutely hilarious. Also most of the enums values returned do not make any sense whatsoever, if you take the documentation at face value... It says Qt::ScrollEnd is only supported on MacOS ? Nope. Qt::NoScrollPhase means the input device does not support scroll phases? Also nope, I get all the enum values depending on the state the application is in... It is completely broken. I have tested this with XFCE and KDE in 5.15.8 and I also left a comment in the Mumble source code... Thanks Qt. What a great framework you are.

\rant

Anyway, I hope this MR now covers all the problems with the slider updates. If there is nothing more to be done here, feel free to merge. This is ready as far as I am concerned.

Krzmbrzl

Small nitpicks. Once they are fixed, feel free to merge this yourself

src/mumble/ListenerVolumeSlider.h

Krzmbrzl · 2023-12-31T13:06:10Z

src/mumble/ListenerVolumeSlider.cpp

+void ListenerVolumeSlider::sendToServer() {
+	ServerHandlerPtr handler = Global::get().sh;
+
+	m_resetTimer.start(3000);


I think 2s would probably also be sufficient. But I'm also fine with leaving this at 3s.

Mh, I have excessively tested all the timings for 3s now. And I really do not feel like going over that all over again with 2s :/

src/mumble/ListenerVolumeSlider.h

src/mumble/widgets/EventFilters.h

Krzmbrzl · 2023-12-31T13:15:20Z

It speaks for you that you endured this silliness with scroll events. My approach would have been to say "well, no scroll support here" 🤷

Previously, when using keyboard arrows or the mouse wheel, the new slider widget action applied the new local volume but did not save it. That was because it used the sliderReleased event to save the value, which is not triggered by keyboard or mouse wheel updates. We could simply save the local volume on every sliderChange instead of on sliderRelease, but that would cause many writes to the disk in a short period of time and possibly lag the client. Additionally, the ListenerVolumeSlider would spam packets to the server, as the listener volume is saved server-side. This commit introduces event filters in addition to the sliderChanged event, which are observing key releases and mouse wheel events. It also introduces a delay timer for packets send to the server from the ListenerVolumeSlider. Fixes mumble-voip#6211

Krzmbrzl · 2023-12-31T15:31:13Z

💚 All backports created successfully

Status	Branch	Result
✅	1.5.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Hartmnt force-pushed the fix_volume_slider_keyboard branch 3 times, most recently from 608226f to 2ee6135 Compare October 11, 2023 19:07

Hartmnt mentioned this pull request Oct 11, 2023

Local volume adjustment levels aren't saved when using keyboard #6211

Closed

Hartmnt added client ui backport-needed bug A bug (error) in the software auto-backport-to-1.5.x labels Oct 11, 2023

Hartmnt requested a review from Krzmbrzl October 11, 2023 19:34

Krzmbrzl reviewed Oct 12, 2023

View reviewed changes

src/mumble/widgets/EventFilters.h Outdated Show resolved Hide resolved

Hartmnt force-pushed the fix_volume_slider_keyboard branch 3 times, most recently from 4c5968a to 5d32ac3 Compare October 12, 2023 17:17

Hartmnt marked this pull request as draft October 15, 2023 22:42

Hartmnt force-pushed the fix_volume_slider_keyboard branch 7 times, most recently from 76e1b95 to e8e0f1b Compare October 16, 2023 20:05

Hartmnt force-pushed the fix_volume_slider_keyboard branch from e8e0f1b to 12b0832 Compare October 20, 2023 12:05

Hartmnt force-pushed the fix_volume_slider_keyboard branch 3 times, most recently from 1c83d7d to 3e769ab Compare October 24, 2023 18:41

Hartmnt force-pushed the fix_volume_slider_keyboard branch from 272d635 to 94be80c Compare October 24, 2023 20:19

Hartmnt force-pushed the fix_volume_slider_keyboard branch 14 times, most recently from 6ea4507 to fba668c Compare November 1, 2023 15:08

Hartmnt marked this pull request as ready for review November 1, 2023 15:22

Hartmnt requested a review from Krzmbrzl November 1, 2023 15:22

Krzmbrzl approved these changes Dec 31, 2023

View reviewed changes

Hartmnt force-pushed the fix_volume_slider_keyboard branch 4 times, most recently from 62cb227 to 208d83c Compare December 31, 2023 14:36

Hartmnt force-pushed the fix_volume_slider_keyboard branch from 208d83c to 6213ed4 Compare December 31, 2023 14:48

Hartmnt merged commit 751ba9b into mumble-voip:master Dec 31, 2023
15 checks passed

Krzmbrzl mentioned this pull request Dec 31, 2023

Backport "Merge pull request #6238 from Hartmnt/fix_volume_slider_keyboard" to 1.5.x #6294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX(client): Fix using the keyboad to change local volume adjustment #6238

FIX(client): Fix using the keyboad to change local volume adjustment #6238

Hartmnt commented Oct 11, 2023

Hartmnt commented Oct 11, 2023

Krzmbrzl left a comment

Hartmnt commented Oct 16, 2023

Hartmnt commented Oct 20, 2023 •

edited

Loading

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 25, 2023

Hartmnt commented Nov 1, 2023

Krzmbrzl left a comment

Krzmbrzl Dec 31, 2023

Hartmnt Dec 31, 2023

Krzmbrzl commented Dec 31, 2023

Krzmbrzl commented Dec 31, 2023

FIX(client): Fix using the keyboad to change local volume adjustment #6238

FIX(client): Fix using the keyboad to change local volume adjustment #6238

Conversation

Hartmnt commented Oct 11, 2023

Hartmnt commented Oct 11, 2023

Krzmbrzl left a comment

Choose a reason for hiding this comment

Hartmnt commented Oct 16, 2023

Hartmnt commented Oct 20, 2023 • edited Loading

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 25, 2023

Hartmnt commented Nov 1, 2023

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl Dec 31, 2023

Choose a reason for hiding this comment

Hartmnt Dec 31, 2023

Choose a reason for hiding this comment

Krzmbrzl commented Dec 31, 2023

Krzmbrzl commented Dec 31, 2023

💚 All backports created successfully

Questions ?

Hartmnt commented Oct 20, 2023 •

edited

Loading